🤖 Kimi Code Review

Security & Correctness Issues

1. Missing signature verification in on_block (store.rs:377-380)

• The PR removes signature verification from on_block but doesn't ensure all callers use precheck_block_signatures. This creates a critical security vulnerability where blocks could bypass signature verification entirely if precheck_block_signatures isn't called first.

Fix: Either:

• Keep signature verification in on_block as a defensive measure, or
• Add #[must_use] to precheck_block_signatures and document that on_block assumes pre-verification

2. Race condition in BlockChainServer (lib.rs:297-305)

• Between the pre-check and process_block, the parent state could change, making the signature verification invalid. The store could be modified by concurrent operations.

Fix: Consider acquiring a lock on the store during both pre-check and processing, or move signature verification into process_block while keeping the early return behavior.

Performance & Design Issues

3. Double state lookup (store.rs:165-170)

• precheck_block_signatures loads the parent state, and on_block will load it again. This is inefficient.

Fix: Consider passing the parent state to on_block after pre-check, or use a cache.

Code Quality

4. Test modification creates inconsistency (signature_spectests.rs:55-65)

• The test now uses pre-check failure as the result, but this doesn't match the actual production flow where on_block might still validate signatures. This could mask real issues.

5. Error handling inconsistency

• precheck_block_signatures returns StoreError but is used in contexts where other error types might be more appropriate.

Recommendations

1. Critical: Add back signature verification to on_block as a defensive measure, even if pre-check passes
2. Performance: Cache the parent state between pre-check and processing
3. Testing: Ensure test flow matches production behavior
4. Documentation: Add clear documentation about the signature verification flow

Automated review by Kimi (Moonshot AI) · custom prompt

🤖 Codex Code Review

Findings

• Critical: store::on_block no longer performs signature verification, and only BlockChainServer::on_block calls the precheck. Callers such as the local proposal path and forkchoice spectests invoke store::on_block directly, so invalid signatures can be accepted into forkchoice/state without any verification. Consider moving the precheck into process_block or store::on_block, or enforce a validated-type wrapper to make skipping impossible. refs: crates/blockchain/src/store.rs:349, crates/blockchain/src/lib.rs:245, crates/blockchain/tests/forkchoice_spectests.rs:58
• Medium: precheck_block_signatures loads parent_state, then on_block loads it again. This is extra work and can (if the store mutates between calls) verify against one state and process against another. Consider returning the parent state from precheck or passing it into on_block to reuse and keep consistency. refs: crates/blockchain/src/store.rs:156, crates/blockchain/src/store.rs:366
• Minor: precheck failures are always logged as “Block signature verification failed,” even when the error is MissingParentState, which is misleading during ops/debugging. Consider matching the error and logging a more accurate message. refs: crates/blockchain/src/lib.rs:297, crates/blockchain/src/store.rs:160

If you want, I can suggest a small refactor to make signature verification mandatory and reuse the parent state lookup.

Automated review by OpenAI Codex · custom prompt

🤖 Claude Code Review

Now I have enough context to provide a comprehensive review. Let me compile my findings:

PR #113 Code Review: Verify block and attestation signatures before processing

Summary

This PR adds signature pre-checking for blocks received from the network before processing them. The main changes move signature verification earlier in the pipeline to fail fast on invalid signatures.

Critical Issues

1. Redundant State Retrieval in precheck_block_signatures (Performance)

Location: crates/blockchain/src/store.rs:161-163

The precheck_block_signatures function retrieves the parent state:

let parent_state = store
    .get_state(&parent_root)
    .ok_or(StoreError::MissingParentState { parent_root, slot })?;

However, on_block() immediately retrieves the same parent state again at line 369-375:

let parent_state =
    store
        .get_state(&block.parent_root)
        .ok_or(StoreError::MissingParentState {
            parent_root: block.parent_root,
            slot,
        })?;

This is a double state retrieval for every valid block. Since states can be large (validator list grows with network size), this is wasteful.

Recommendation: Consider one of these approaches:

1. Have precheck_block_signatures return the parent state, so on_block can reuse it
2. Accept the performance trade-off for cleaner separation of concerns (document this decision)
3. Merge the signature check into the beginning of on_block() itself

2. Missing Signature Verification for Self-Produced Blocks

Location: crates/blockchain/src/lib.rs:263-274

The process_block() function is called from two paths:

1. on_block() - external blocks from network → NOW has signature precheck ✅
2. propose_block() → process_block() - self-produced blocks → NO signature precheck ❌

Self-produced blocks bypass the signature verification at line 298-301 because they go directly through process_block() without the precheck.

Security Impact: While self-produced blocks should be valid, defense-in-depth suggests verifying them too. This could catch:

• Bugs in block building logic
• Key manager issues
• Signature aggregation failures

Recommendation: Either:

1. Add the same precheck in propose_block() after building the block
2. Document why self-produced blocks are trusted (if intentional)

3. Attestations from Blocks Still Lack Signature Verification

Location: crates/blockchain/src/store.rs:420-428

The TODO comment at line 421 remains:

// TODO: validate attestations before processing
let _ = on_attestation(store, attestation, true)

While the PR title mentions "verify block and attestation signatures," attestation signatures within block bodies are still unverified before being processed into fork choice. The verify_signatures() function verifies them, but this happens in precheck_block_signatures(), which only logs errors—it doesn't prevent the attestations from being processed.

Wait, let me re-check this logic...

Actually, looking at the code flow:

1. precheck_block_signatures() calls verify_signatures() which verifies attestation proofs
2. If verification fails, the entire block is rejected at line 298-301 before on_block() is called
3. Therefore, by the time we reach line 422, the attestations have already been verified

So this is actually CORRECT. The TODO comment is now misleading since attestations ARE verified via the precheck. The error handling at line 422-427 is just for structural/temporal validation errors.

Recommendation: Update or remove the TODO comment at line 421 since attestations are now verified before reaching this point.

4. Proposer Attestation Signature Verification Remains Unchanged

Location: crates/blockchain/src/store.rs:452-459

The proposer attestation (line 453-459) has a similar TODO comment:

// TODO: validate attestations before processing
let _ = on_attestation(store, proposer_attestation, false)

However, the proposer attestation signature IS verified by verify_signatures() at line 1063-1087 (checking the proposer attestation signature separately). So this TODO is also now outdated.

Recommendation: Update or remove the TODO at line 453.

Minor Issues

5. Test Logic Could Be Simplified

Location: crates/blockchain/tests/signature_spectests.rs:55-64

The test logic is awkward:

let precheck_result = store::precheck_block_signatures(&st, &signed_block);

let result = if precheck_result.is_err() {
    precheck_result
} else {
    store::on_block(&mut st, signed_block)
};

Recommendation: Use .and_then() for cleaner flow:

let result = store::precheck_block_signatures(&st, &signed_block)
    .and_then(|_| store::on_block(&mut st, signed_block));

6. Inconsistent Error Context

Location: crates/blockchain/src/lib.rs:298-300

The error log at line 299 doesn't include the block root, while the success log at line 309 does. This makes debugging harder when signatures fail.

Recommendation:

warn!(%slot, block_root = %ShortRoot(&block_root.0), %err, "Block signature verification failed");

Positive Observations

✅ Correct use of cfg!(feature = "skip-signature-verification") - properly skips verification in test mode
✅ Good separation of concerns - precheck_block_signatures is a clean, testable function
✅ Proper error propagation - signature failures correctly reject the entire block
✅ Documentation - function docstrings clearly explain the purpose
✅ Metrics preserved - signature verification metrics still tracked properly

Summary of Recommendations

High Priority:

1. Verify self-produced blocks OR document why they're trusted (Update: checking the code, self-produced blocks go through the same process_block path, so they'd need explicit handling)
2. Consider performance impact of double state retrieval (benchmark if needed)

Medium Priority:
3. Remove/update misleading TODO comments at lines 421 and 453 (attestations ARE now verified)
4. Add block_root to signature verification failure log

Low Priority:
5. Simplify test logic using .and_then()

Verdict

The PR successfully addresses the signature verification issue for external blocks. The main concern is the redundant state retrieval which could be optimized. The TODO comments should be updated since attestation signatures ARE now being verified before processing (just at a different point in the pipeline).

Approval Status: ✅ Approved with suggestions
Risk Level: Low (mostly performance optimization opportunities)

Automated review by Claude (Anthropic) · custom prompt

Greptile Overview

Greptile Summary

This PR factors block signature verification into a new store::precheck_block_signatures() helper and uses it as an early gate before importing gossiped blocks. Signature verification logic was removed from store::on_block() and the signature spec test harness was updated to call the new precheck before processing fixtures.

The main integration points are:

• BlockChainServer::on_block() now runs the precheck before calling process_block().
• store::on_block() performs state transition and forkchoice updates but no longer verifies block signatures itself.

Confidence Score: 2/5

• Not safe to merge as-is due to a signature verification invariant regression.
• Signature verification was removed from the core store::on_block() entrypoint and is now enforced only by some callers. Any direct call to on_block() without a precheck will import blocks without cryptographic validation, which is a correctness/security regression for consensus processing.
• crates/blockchain/src/store.rs and crates/blockchain/src/lib.rs

Sequence Diagram

sequenceDiagram
    participant P2P as P2P Layer
    participant BC as BlockChainServer (lib.rs)
    participant ST as store.rs
    participant FS as Forkchoice Store

    P2P->>BC: NewBlock(SignedBlockWithAttestation)
    BC->>BC: check parent exists
    alt parent missing
        BC->>P2P: FetchBlock(parent_root)
        BC->>BC: queue as pending
    else parent exists
        BC->>ST: precheck_block_signatures(&Store, &signed_block)
        alt signatures invalid
            ST-->>BC: Err(StoreError)
            BC-->>P2P: (drop block)
        else signatures ok
            ST-->>BC: Ok(())
            BC->>ST: on_block(&mut Store, signed_block)
            ST->>FS: state_transition + insert block/state
            ST->>FS: update_head + process attestations
            ST-->>BC: Ok(())
        end
    end

    Note over BC,ST: Local proposal path currently calls store::on_block() via process_block() without precheck